Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-598. Support get environment list from database #374

Closed
wants to merge 10 commits into from
Closed

SUBMARINE-598. Support get environment list from database #374

wants to merge 10 commits into from

Conversation

kobe860219
Copy link
Contributor

@kobe860219 kobe860219 commented Aug 11, 2020 •
edited
Loading

What is this PR for?

Improve environment swagger api.
When cache is null. Will get environment data from database.

What type of PR is it?

[Improvement]

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/SUBMARINE-598

How should this be tested?

https://travis-ci.org/github/kobe860219/submarine/builds/716904008?utm_source=github_status&utm_medium=notification

Screenshots (if appropriate)

截圖 2020-08-16 上午12 11 07

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@kobe860219 kobe860219 changed the title Submarine-598. Support get environment list from database SUBMARINE-598. Support get environment list from database Aug 11, 2020
@kobe860219
Copy link
Contributor Author

@liuxunorg @pingsutw @JohnTing Please help me review this pr. Thanks :)

jiwq
jiwq reviewed Aug 11, 2020
View reviewed changes
Copy link
Member

@jiwq jiwq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kobe860219 for the contribution.

...r/server-core/src/main/resources/org/apache/submarine/database/mappers/EnvironmentMapper.xml Outdated Show resolved Hide resolved
...er/server-core/src/main/java/org/apache/submarine/server/environment/EnvironmentManager.java Outdated Show resolved Hide resolved
...er/server-core/src/main/java/org/apache/submarine/server/environment/EnvironmentManager.java Outdated Show resolved Hide resolved
...er/server-core/src/main/java/org/apache/submarine/server/environment/EnvironmentManager.java Outdated Show resolved Hide resolved
@kobe860219 kobe860219 requested review from jiwq and pingsutw August 12, 2020 11:34
xunliu
xunliu reviewed Aug 12, 2020
View reviewed changes
...er/server-core/src/main/java/org/apache/submarine/server/environment/EnvironmentManager.java Outdated Show resolved Hide resolved
...er/server-core/src/main/java/org/apache/submarine/server/environment/EnvironmentManager.java Outdated
return environmentList;

if (envs.size() != 0) {
return envs;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If direct return envs, Will this cause envs variable to never update?

@kobe860219
Copy link
Contributor Author

kobe860219 commented Aug 12, 2020 •
edited
Loading

@jiwq @liuxunorg @pingsutw @JohnTing I updated this pr and screenshots. I add a path /list that support get environments data from database by /v1/environment/list . It is able to support environment page of workbench show environments list from database.

@kobe860219 kobe860219 requested a review from xunliu August 12, 2020 14:19
pingsutw
pingsutw reviewed Aug 12, 2020
View reviewed changes
...ne-server/server-core/src/main/java/org/apache/submarine/server/rest/EnvironmentRestApi.java Outdated
content = @Content(
schema = @Schema(
implementation = Environment.class)))})
public Response listAllEnvironments() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a listEnvironment in EnvironmentRestApi.java, why create a new function listAllEnvironments?

submarine/submarine-server/server-core/src/main/java/org/apache/submarine/server/rest/EnvironmentRestApi.java

Lines 151 to 159 in cc9dde5

public Response listEnvironment(@QueryParam("status") String status) {
try {
List<Environment> environmentList =
environmentManager.listEnvironments(status);
return new JsonResponse.Builder<List<Environment>>(Response.Status.OK)
.success(true).result(environmentList).build();
} catch (SubmarineRuntimeException e) {
return parseEnvironmentServiceException(e);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listEnvironment just get those environments in cache. listAllEnvironments will get environments from database.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can merge it rather than create new.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will merge it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kobe860219 Thanks for working on this. Even from API end point perspective, I don't see a need for "list". Just typing "/v1/environment" should return the list of environments. Change is required only in model layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiwq @pingsutw @manirajv06 I have updated this pr. Please take a look, Thanks :))

@kobe860219 kobe860219 requested a review from pingsutw August 13, 2020 01:56
@pingsutw pingsutw mentioned this pull request Aug 14, 2020
Closed
@jiwq
Copy link
Member

jiwq commented Aug 17, 2020

@kobe860219 Could you fix the failed tests: EnvironmentRestApiTest.listEnvironment:129 expected:<1> but was:<2>? https://travis-ci.org/github/apache/submarine/jobs/718209689

@kobe860219
Copy link
Contributor Author

@jiwq I have fixed the failed tests :)

@jiwq
Copy link
Member

jiwq commented Aug 18, 2020

+1 Will commit if no more comments.

xunliu
xunliu approved these changes Aug 19, 2020
View reviewed changes
Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@asfgit asfgit closed this in 689ae3c Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@manirajv06 manirajv06 manirajv06 left review comments

@xunliu xunliu xunliu approved these changes

@jiwq jiwq Awaiting requested review from jiwq

@pingsutw pingsutw Awaiting requested review from pingsutw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kobe860219 @jiwq @manirajv06 @xunliu @pingsutw